Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix a DST error in date_histogram (backport of #52016) #52294

Merged
merged 1 commit into from
Feb 13, 2020

Conversation

nik9000
Copy link
Member

@nik9000 nik9000 commented Feb 12, 2020

When date_histogram attempts to optimize itself it for a particular
time zone it checks to see if the entire shard is within the same
"transition". Most time zone transition once every size months or
thereabouts so the optimization can usually kicks in.

But it crashes when you attempt feed it a time zone who's last DST
transition was before epoch. The reason for this is a little twisted:
before this patch it'd find the next and previous transitions in
milliseconds since epoch. Then it'd cast them to Longs and pass them
into the DateFieldType to check if the shard's contents were within
the range. The trouble is they are then converted to Strings which are
then parsed back to Instants which are then convertd to longs. And
the parser doesn't like most negative numbers. And everything before
epoch is negative.

This change removes the
long -> Long -> String -> Instant -> long chain in favor of
passing the long -> Instant -> long which avoids the fairly complex
parsing code and handles a bunch of interesting edge cases around
epoch. And other edge cases around date_nanos.

Closes #50265

When `date_histogram` attempts to optimize itself it for a particular
time zone it checks to see if the entire shard is within the same
"transition". Most time zone transition once every size months or
thereabouts so the optimization can usually kicks in.

*But* it crashes when you attempt feed it a time zone who's last DST
transition was before epoch. The reason for this is a little twisted:
before this patch it'd find the next and previous transitions in
milliseconds since epoch. Then it'd cast them to `Long`s and pass them
into the `DateFieldType` to check if the shard's contents were within
the range. The trouble is they are then converted to `String`s which are
*then* parsed back to `Instant`s which are then convertd to `long`s. And
the parser doesn't like most negative numbers. And everything before
epoch is negative.

This change removes the
`long` -> `Long` -> `String` -> `Instant` -> `long` chain in favor of
passing the `long` -> `Instant` -> `long` which avoids the fairly complex
parsing code and handles a bunch of interesting edge cases around
epoch. And other edge cases around `date_nanos`.

Closes elastic#50265
@nik9000 nik9000 merged commit f6cc48a into elastic:7.6 Feb 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant